Skip to content

[wip] fd commands #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[wip] fd commands #1

wants to merge 11 commits into from

Conversation

@leongross
Copy link
Member

@leongross leongross commented Nov 6, 2025 •
edited
Loading

Roadmap

  • Add struct definitions for
    • QueryDownstreamDevices (dc25fdf)
      • test_query_downstream_devices_request_codec
      • test_query_downstream_devices_response_codec
    • QueryDownstreamIdentifiers (dc25fdf)
      • test_query_downstream_identifiers_request_codec
      • test_query_downstream_identifiers_response_codec
    • GetDownstreamFirmwareParameters (b63b0f6)
      • test_get_downstream_firmware_parameters_request_codec
      • test_get_downstream_firmware_parameters_response_codec
      • test_device_parameter_table_codec
      • test_get_downstream_firmware_parameters_portion_codec
    • RequestDownstreamDeviceUpdate (69aac44)
      • test_request_downstream_device_update_request_codec
      • test_request_downstream_device_update_response_codec
    • GetPackageData (ab8f458)
      • test_get_package_data_request_codec
      • test_get_package_data_response_codec
    • GetDeviceMetaData (1face4e)
      • test_get_device_meta_data_request_codec
      • test_get_device_meta_data_response_codec
    • GetMetaData (116aebd)
      • test_get_metadata_request_codec
      • test_get_meta_data_response_codec
  • Add macro definition for response types (9423913)
  • Add/replace response types for remaining command responses (c42fcbe)
  • Replace const size portion buffers with dynamically sized buffers
  • Testing
    • Add unit tests for all command requests and responses
    • Fix existing, broken unit tests
      • test_query_device_identifiers_resp_codec
      • test_request_firmware_data_response_codec
    • Add program flow tests similar to the examples in the DMTF documentation
    • Coverage: 67.67% coverage, 1080/1596 lines covered

@leongross
fd-commands: add QueryDownstreamDevices and QueryDownstreamIdentifiers
dc25fdf 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add GetDownstreamFirmwareParameters
b63b0f6 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add RequestDownstreamDeviceUpdate
69aac44 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add GetPackageData
ab8f458 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add GetDeviceMetaData
1face4e 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add GetMetaData
116aebd 
Signed-off-by: leongross <[email protected]>
@leongross
fd-commands: add pldm_completion_code
9423913 
The results for the corresponding pldm requests have an u8 field
called CompletionCode. These completion codes are typically a
set of PLDM_BASE_CODES and a selection of FW_UPDATE codes.
To pass these return codes in a generic and type safe way, we
want to define enums that implement the `From` trait for u8,
individually for each response.

Signed-off-by: leongross <[email protected]>
@leongross leongross force-pushed the leongross/fd-commands branch from c42fcbe to a8594d2 Compare November 7, 2025 11:08
@leongross leongross force-pushed the leongross/fd-commands branch 4 times, most recently from ecd1727 to 2ef891d Compare November 12, 2025 16:49
@leongross leongross force-pushed the leongross/fd-commands branch 4 times, most recently from 18eb056 to bcf45fe Compare November 14, 2025 16:43
embediver
embediver reviewed Nov 19, 2025
View reviewed changes
src/message/firmware_update/query_downstream.rs
Comment on lines 1373 to 1467
assert!(dsd_iter.is_some());

Copy link

@embediver embediver Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking that we actually get the correct descriptors would be nice.
A few more asserts to check the first descriptor field for every descriptor should be enough.

@leongross leongross force-pushed the leongross/fd-commands branch 6 times, most recently from a652e60 to b225fbe Compare November 20, 2025 15:47
@leongross leongross requested a review from Copilot November 20, 2025 16:11
Copilot finished reviewing on behalf of leongross November 20, 2025 16:15
Copilot AI reviewed Nov 20, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This WIP PR adds support for firmware device (FD) commands as defined in DSP0267, implementing several downstream device management commands for the PLDM firmware update protocol. The implementation includes new command structures, a reusable completion code macro, and comprehensive test coverage.

Key Changes:

  • Introduces 7 new FD commands: QueryDownstreamDevices, QueryDownstreamIdentifiers, GetDownstreamFirmwareParameters, RequestDownstreamDeviceUpdate, GetPackageData, GetDeviceMetaData, and GetMetaData
  • Adds pldm_completion_code! macro for type-safe completion code handling with base and custom codes
  • Implements PldmCodecWithLifetime trait for structures that need to borrow from decode buffers

Reviewed Changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 20 comments.

Show a summary per file
File Description
src/protocol/firmware_update.rs Adds new command enums, completion codes, KnownLayout trait to Descriptor, custom PldmCodec for PldmFirmwareString, and updates FirmwareDeviceCapability bitfield
src/protocol/base.rs Introduces pldm_completion_code! macro for creating type-safe completion code enums and adds PartialEq to PldmBaseCompletionCode
src/message/firmware_update/query_downstream.rs New module implementing downstream device query commands with iterators for parsing variable-length response data
src/message/firmware_update/get_package_data.rs New module implementing package data and metadata retrieval commands
src/message/firmware_update/mod.rs Registers new query_downstream and get_package_data modules
src/message/firmware_update/request_fw_data.rs Migrates to PldmCodecWithLifetime trait and implements decode functionality
src/message/firmware_update/*.rs Standardizes test naming convention with _codec suffix across multiple test files
src/codec.rs Adds PldmCodecWithLifetime trait for lifetime-aware codec operations and InvalidData error variant
Cargo.toml / Cargo.lock Adds bytemuck dependency (appears unused)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@leongross leongross force-pushed the leongross/fd-commands branch 2 times, most recently from d0bb060 to 8d1b62f Compare November 20, 2025 17:21
@leongross leongross force-pushed the leongross/fd-commands branch from 8d1b62f to 9c282ea Compare November 24, 2025 14:16
@leongross leongross marked this pull request as ready for review November 24, 2025 14:19
@leongross leongross requested a review from embediver November 24, 2025 14:19
embediver
embediver reviewed Nov 25, 2025
View reviewed changes
Copy link

@embediver embediver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also two unresolved item links in the doc and the doc could get some more love in general.

But lets do the upstream PR now, we can fix the things I pointed out afterwards.

src/protocol/base.rs
/// This macro is useful for the completion codes for the command responses, to ensure
/// type safety while still allowing the use of base completion codes and an
/// arbitrary number of custom completion codes specific to the command.
#[macro_export]
Copy link

@embediver embediver Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be exported?
(macro_export exports (pub) the macro at the root level of the crate for everyone)

The alternative is to macro_use it so it is available globally in this crate.
(Macro scopes are special...)

src/message/firmware_update/query_downstream.rs
Comment on lines +648 to +651
/// Wrapper struct for PLDM timestamp representation
#[derive(Debug, Clone, Copy, PartialEq, FromBytes, IntoBytes)]
#[repr(C, packed)]
pub struct PldmTimeStamp([u8; 8]);
Copy link

@embediver embediver Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move this into another module imo, since its a base PLDM data type.
(protocol::base probably)

@leongross leongross force-pushed the leongross/fd-commands branch from 9c282ea to c32abc4 Compare November 25, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@embediver embediver embediver left review comments

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@leongross @embediver